Skip to content

test: expand assert_no_critical_errors to also allow checking other metrics + add a default check in system tests that no consensus artifacts were invalidated #8815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
kpop-dfinity merged 7 commits into from
Feb 12, 2026
Merged

test: expand assert_no_critical_errors to also allow checking other metrics + add a default check in system tests that no consensus artifacts were invalidated #8815

kpop-dfinity merged 7 commits into from
Feb 12, 2026
+43 −22

Conversation

@kpop-dfinity
Copy link
Contributor

@kpop-dfinity kpop-dfinity commented Feb 12, 2026 •
edited
Loading

Failing test output example:

Task assert_no_metrics_errors        FAILED  in   0.35s
     assertion `left == right` failed: The metric `consensus_invalidated_artifacts` on node 2kknl-owq7r-fbyry-fo3yj-hlj6w-7nx4y-4ch53-a4tov-wudg2-eatoz-qae has non-zero value. If the metric is allow
ed to be non-zero in the test, Create `SystemTestGroup` with `remove_metrics_to_check("consensus_invalidated_artifacts")
       left: 141
      right: 0

@github-actions github-actions bot added the test label Feb 12, 2026
@kpop-dfinity kpop-dfinity marked this pull request as ready for review February 12, 2026 12:30
@kpop-dfinity kpop-dfinity requested review from a team as code owners February 12, 2026 12:30
basvandijk
basvandijk approved these changes Feb 12, 2026
View reviewed changes
Copy link
Collaborator

@basvandijk basvandijk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. LGTM!

@basvandijk basvandijk requested a review from mraszyk February 12, 2026 12:35
eichhorl
eichhorl approved these changes Feb 12, 2026
View reviewed changes
Copy link
Contributor

@eichhorl eichhorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively it could be checked by default with an option to disable it?

pierugo-dfinity
pierugo-dfinity reviewed Feb 12, 2026
View reviewed changes
@kpop-dfinity
Copy link
Contributor Author

kpop-dfinity commented Feb 12, 2026

Alternatively it could be checked by default with an option to disable it?

Good idea, let me do that

@kpop-dfinity kpop-dfinity added the CI_ALL_BAZEL_TARGETS Runs all bazel targets and uploads them to S3 label Feb 12, 2026
@kpop-dfinity
Copy link
Contributor Author

kpop-dfinity commented Feb 12, 2026

@basvandijk @eichhorl @pierugo-dfinity
Could you take a look again? I opened the PR prematurely.
As Leo suggested I made the consensus_invalidated_artifacts metric a default metric to check.

@kpop-dfinity kpop-dfinity changed the title test: expand assert_no_critical_errors to also allow checking other metrics + add a check to several consensus system tests that no artifacts where invalidated test: expand assert_no_critical_errors to also allow checking other metrics + add a default check in system tests that no consensus artifacts where invalidated Feb 12, 2026
@kpop-dfinity kpop-dfinity changed the title test: expand assert_no_critical_errors to also allow checking other metrics + add a default check in system tests that no consensus artifacts where invalidated test: expand assert_no_critical_errors to also allow checking other metrics + add a default check in system tests that no consensus artifacts were invalidated Feb 12, 2026
@kpop-dfinity kpop-dfinity added this pull request to the merge queue Feb 12, 2026
Merged via the queue into master with commit 8d41fa6 Feb 12, 2026
39 checks passed
@kpop-dfinity kpop-dfinity deleted the kpop/tests/add_more_validations_for_system_tests branch February 12, 2026 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@basvandijk basvandijk basvandijk approved these changes

@mraszyk mraszyk mraszyk approved these changes

@eichhorl eichhorl eichhorl approved these changes

@pierugo-dfinity pierugo-dfinity pierugo-dfinity approved these changes

Assignees

No one assigned

Labels

CI_ALL_BAZEL_TARGETS Runs all bazel targets and uploads them to S3 @consensus @idx test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kpop-dfinity @basvandijk @mraszyk @eichhorl @pierugo-dfinity